Conversation
CMake discoverability
There was a problem hiding this comment.
Thanks @VictorEijkhout for contributing this! Looks good to me!
@crtrott What do you think?
|
@VictorEijkhout Btw I took the liberty of adding a "Fixes #339" message to your PR description, so that merging this PR will automatically close the issue. |
crtrott
left a comment
crtrott
left a comment
There was a problem hiding this comment.
I had yesterday a discussion with Damien on this. This PR points something unfortunate out: I am not sure that the target being called std::mdspan is a very fortunate choice considering that mdspan is in the standard, and part of LLVM for example. We were wondering if we should rename the target (with providing an alias as std::mdspan for a while).
|
@crtrott I had the same thought. Especially since in the program it's still |
nmm0
left a comment
nmm0
left a comment
There was a problem hiding this comment.
Thanks for submitting this! I just have a couple of suggestions
| * renaming `pointer`, `data`, `is_contiguous` and `is_always_contiguous`; and before | ||
| * renaming `size_type` to `index_type` and introducing a new `size_type = make_unsigned_t<index_type>` alias. | ||
|
|
||
| Building code with `mdspan` |
There was a problem hiding this comment.
I think we should move this under the "Using mdspan" subheading and retitle this something like "Cmake find_package" or something like that
|
|
||
| mdspan is discoverable by CMake: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
| ``` | |
| ```cmake |
| ``` | ||
| #include "mdspan/mdspan.hpp" | ||
| namespace md = Kokkos; | ||
| ... | ||
| md::mdspan |
There was a problem hiding this comment.
I think I would just have the include here (I personally prefer with angle brackets, but I suppose it's a matter of taste)
| ``` | |
| #include "mdspan/mdspan.hpp" | |
| namespace md = Kokkos; | |
| ... | |
| md::mdspan | |
| ```cpp | |
| #include <mdspan/mdspan.hpp> |
I think we could probably leave out the namespace alias here
|
We should take this over and change what we name our cmake targets |
4 participants
CMake discoverability
Fixes #339